Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Updates qemu framework to install pip requirements from its own requirements file #14355

Merged
merged 1 commit into from
Mar 7, 2019
Merged

Updates qemu framework to install pip requirements from its own requirements file #14355

merged 1 commit into from
Mar 7, 2019

Conversation

perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Mar 7, 2019 •
edited
Loading

Description

Removes the installation of tests/requirements.txt. If this file contains requirements for which there are no existing Raspberry Pi wheel files, a build from source will be triggered. This can cause CI timeouts. From now on, the python requirements for qemu tests should to be managed separately.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@edisongustavo
Copy link
Contributor

Very interesting findings.

@perdasilva
Copy link
Contributor Author

@marcoabreu could you please merge this? ^^

@marcoabreu
Copy link
Contributor

I don't agree with the approach because it removes transparency and instead makes the project rely on a provided blob (aka the OS image used in the Emulator). Pedro chose this approach specifically to keep up reproducibility.

From what I can tell, we are trying to circumvent a different problem here, which is that windows as well as qemu use the same test requirements.

So I'd either propose to generate a new OS image (preferred solution) or split the requirements.

@perdasilva
Copy link
Contributor Author

perdasilva commented Mar 7, 2019 •
edited
Loading

I agree that this is the preferred solution. But, a few things to keep in mind:

  • this is blocking the windows tests
  • it takes a lifetime to compile scipy in rasp py
  • there is no automation around creating the image
  • the one test we run inside the emulated box does not require scipy
  • we get some transparency by specifying that the mxnet_requirements.txt are what's in the box
  • with the addition of arm instances on aws, it's highly likely we will refactor this approach anyway

@lebeg
Copy link
Contributor

lebeg commented Mar 7, 2019

Good points @perdasilva

@gavinmbell
Copy link

For the sake of maintaining the mental model for future eyeballs to look at. Leaving breadcrumbs in expected places with the right information to show the err of this way makes for a cleaner knowledge trail. Though it is not pragmatic... it is keeping good hygiene for the future.

download

@perdasilva perdasilva force-pushed the qemu_test_requirements branch from d10a9cf to ec5f6a0 Compare March 7, 2019 12:57
@perdasilva
Copy link
Contributor Author

perdasilva commented Mar 7, 2019 •
edited
Loading

@gavinmbell @marcoabreu I've changed the approach slightly. Is this more agreeable?

re. "From what I can tell, we are trying to circumvent a different problem here, which is that windows as well as qemu use the same test requirements." - actually, some of the tests require numpy and scipy. This issue just wasnt caught before because the test requirements are also installed in the the linux docker environments in install/ubuntu_python.sh (which in this case include scipy).

I guess, previously, the windows ami also pip installed these dependencies, which is why it wasnt caught earlier. Idk.

gavinmbell
gavinmbell reviewed Mar 7, 2019
View reviewed changes
ci/qemu/README.md Outdated

# CI and Testing

Formally, [runtime_functions.py](https://github.com/apache/incubator-mxnet/blob/master/ci/docker/qemu/runtime_functions.py) would [run](https://github.com/apache/incubator-mxnet/blob/8beea18e3d9835f90b59d3f9de8f9945ac819423/ci/docker/qemu/runtime_functions.py#L81) *pip install -r [mxnet/tests/requirements.txt](https://github.com/apache/incubator-mxnet/blob/master/tests/requirements.txt)*. If the requirements change, there can be an unfortunate side-effect that there are no wheel files for Raspberry Pi for the new requirement. This would trigger a build from source on the emulator, which can take a long time and cause job timeouts. Therefore, we no longer install the `tests/requirements.txt` requirements, but rather rely on [mxnet-requirements.txt](https://github.com/apache/incubator-mxnet/blob/master/ci/qemu/mxnet_requirements.txt) to maintain the requirements for the qemu tests. Should any requirements changes lead to a job time out, it is incumbent on the submitter to update the image to include the requirement and unblock ci.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good... So at least there is a pointer for folks to know what does NOT work!
👍

marcoabreu
marcoabreu approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks :)

@marcoabreu
Copy link
Contributor

Nit: You might wanna adapt the title.

@gavinmbell
Copy link

Lets put this to bed Mr nit :-)
im_out_mic_drop

@perdasilva perdasilva force-pushed the qemu_test_requirements branch from ec5f6a0 to 5fa2b8a Compare March 7, 2019 13:15
@perdasilva perdasilva changed the title Removes test requirements installation from qemu tests Updates qemu framework to install pip requirements from its own requirements file Mar 7, 2019
@perdasilva perdasilva force-pushed the qemu_test_requirements branch from 5fa2b8a to 8633a51 Compare March 7, 2019 14:21
@vandanavk
Copy link
Contributor

@mxnet-label-bot add [CI, pr-awaiting-review]

@perdasilva could you have a look at the CI failure?

@marcoabreu marcoabreu added CI pr-awaiting-review PR is waiting for code review labels Mar 7, 2019
@perdasilva perdasilva force-pushed the qemu_test_requirements branch from 8633a51 to 9949e5b Compare March 7, 2019 15:28
@perdasilva perdasilva force-pushed the qemu_test_requirements branch from 9949e5b to 02200cd Compare March 7, 2019 15:41
@perdasilva perdasilva mentioned this pull request Mar 7, 2019
Merged
5 tasks
@apeforest apeforest merged commit 2b7d57d into apache:master Mar 7, 2019
perdasilva added a commit to perdasilva/incubator-mxnet that referenced this pull request Mar 13, 2019
@perdasilva
Installs qemu pip requirements from qemu requirements file (apache#14355
cbc3f79 
)
@perdasilva perdasilva mentioned this pull request Mar 14, 2019
Merged
5 tasks
marcoabreu pushed a commit that referenced this pull request Mar 14, 2019
@perdasilva @marcoabreu
v1.4.x: Backport of Windows CI Fixes (#14420)
44265aa 
* Backports Windows CI pipeline fixes from master

* Updates Jenkins file to use python 3 when calling build_windows.py

* Installs qemu pip requirements from qemu requirements file (#14355)

* remove a white space
vdantu pushed a commit to vdantu/incubator-mxnet that referenced this pull request Mar 31, 2019
@perdasilva @vdantu
Installs qemu pip requirements from qemu requirements file (apache#14355
6fae534 
)
nswamy pushed a commit that referenced this pull request Apr 5, 2019
@perdasilva @nswamy
Installs qemu pip requirements from qemu requirements file (#14355)
0b3a965
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@perdasilva
Installs qemu pip requirements from qemu requirements file (apache#14355
b7fdc65 
)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marcoabreu marcoabreu marcoabreu approved these changes

@gavinmbell gavinmbell gavinmbell approved these changes

@edisongustavo edisongustavo edisongustavo approved these changes

Assignees
No one assigned
Labels
CI pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@perdasilva @edisongustavo @marcoabreu @lebeg @gavinmbell @vandanavk @apeforest